Skip to content

CNTRLPLANE-1388: Spot with termination handler - #7567

Merged
openshift-merge-bot[bot] merged 3 commits into
openshift:mainfrom
enxebre:spot-with-termination-handler
Jan 27, 2026
Merged

CNTRLPLANE-1388: Spot with termination handler#7567
openshift-merge-bot[bot] merged 3 commits into
openshift:mainfrom
enxebre:spot-with-termination-handler

Conversation

@enxebre

@enxebre enxebre commented Jan 22, 2026

Copy link
Copy Markdown
Member

What this PR does / why we need it:

Implement control plane operator v2 component for AWS node termination
handler with MachineHealthCheck integration.

Components added:

  • AWS node termination handler Deployment for spot interruption handling
  • AWS credentials secret with NodePoolManagement role ARN
  • MachineHealthCheck for spot instances with interruptible-instance label
  • Component predicate checking for AWS platform and SQS queue URL annotation
  • interruptible-instance label propagation to MachineDeployment templates and existing Machines

The component is deployed when the HostedCluster has:

  • AWS platform type
  • Annotation: hypershift.openshift.io/aws-termination-handler-queue-url

Features:

  • SQS queue monitoring for spot interruption events
  • Automatic node draining on spot interruption warnings
  • Machine health checking for interrupted instances
  • Web identity token authentication using NodePoolManagement ARN

TODOs:

  • Replace aws-node-termination-handler image with mirror image or payload once available
  • Remove SQS queue URL annotation once the input is in the HostedCluster AWS API
  • Replace hardcoded constants with API based configuration

Which issue(s) this PR fixes:

Fixes

Special notes for your reviewer:

Checklist:

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

@openshift-ci

openshift-ci Bot commented Jan 22, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot

openshift-ci-robot commented Jan 22, 2026

Copy link
Copy Markdown

@enxebre: This pull request references CNTRLPLANE-1388 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set.

Details

In response to this:

What this PR does / why we need it:

Implement control plane operator v2 component for AWS node termination
handler with MachineHealthCheck integration.

Components added:

  • AWS node termination handler Deployment for spot interruption handling
  • AWS credentials secret with NodePoolManagement role ARN
  • MachineHealthCheck for spot instances with interruptible-instance label
  • Component predicate checking for AWS platform and SQS queue URL annotation
  • interruptible-instance label propagation to MachineDeployment templates and existing Machines

The component is deployed when the HostedCluster has:

  • AWS platform type
  • Annotation: hypershift.openshift.io/aws-termination-handler-queue-url

Features:

  • SQS queue monitoring for spot interruption events
  • Automatic node draining on spot interruption warnings
  • Machine health checking for interrupted instances
  • Web identity token authentication using NodePoolManagement ARN

TODOs:

  • Replace aws-node-termination-handler image with mirror image or payload once available
  • Remove SQS queue URL annotation once the input is in the HostedCluster AWS API
  • Replace hardcoded constants with API based configuration

Which issue(s) this PR fixes:

Fixes

Special notes for your reviewer:

Checklist:

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 22, 2026
@openshift-ci openshift-ci Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-area labels Jan 22, 2026
@coderabbitai

coderabbitai Bot commented Jan 22, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Adds an AWS Node Termination Handler: new v2 control-plane component, deployment and secret assets, SQS IAM and client wiring, HostedCluster/HostedControlPlane annotation propagation and disable flag, NodePool spot support with spot-specific MachineHealthChecks, unit and e2e tests, and multiple test fixture updates.

Changes

Cohort / File(s) Summary
API Types
api/hypershift/v1beta1/hostedcluster_types.go
Added new public constant DisableAWSNodeTerminationHandlerAnnotation = "hypershift.openshift.io/disable-aws-node-termination-handler".
Infra IAM & AWS Delegation
cmd/infra/aws/iam.go, cmd/infra/aws/delegating_client.go
Added SQS IAM permissions (sqs:DeleteMessage, sqs:ReceiveMessage) to node-pool policy and wired SQS into delegating AWS client (new sqsClient type, DelegatingClient exposes SQS API).
Control Plane Operator — Component Registration
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
Registered awsnodeterminationhandlerv2.NewComponent() in HostedControlPlane component list.
Control Plane Operator — Component Implementation (v2)
control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/...
.../component.go, .../deployment.go, .../secret.go, .../deployment_test.go
New component with exported NewComponent(), constants (component name, annotation key, default image), predicate gating (AWS platform, RolesRef.NodePoolManagementARN, queue-url annotation, kubeconfig, secret), deployment adaptor (env, token-audience, replica scaling via disable annotation), credentials secret adaptor, and unit tests.
Control Plane Operator — Assets & Test Fixtures
control-plane-operator/controllers/hostedcontrolplane/v2/assets/aws-node-termination-handler/*, control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/...
Added deployment and credentials secret assets and many platform-specific test fixtures (ControlPlaneComponent, Secret, Deployment YAMLs) for aws-node-termination-handler.
Control Plane Operator — Other Testdata Updates
control-plane-operator/controllers/hostedcontrolplane/testdata/* (cluster-image-registry-operator, cluster-network-operator, kube-apiserver, openshift-oauth-apiserver, etc.)
Replaced empty token-audience values with a concrete OIDC URL across multiple fixtures, updated KAS ConfigMap OIDC fields, added ConfigMap metadata (namespace, ownerReferences, resourceVersion), and adjusted several config-hash annotation values.
HostedCluster Controller
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go, hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
Added isAWSNodeTerminationHandlerNeeded(ctx,hcluster) to detect spot nodepools; propagated awsNodeTerminationNeeded through reconciliation; updated reconcileHostedControlPlane and reconcileHostedControlPlaneAnnotations signatures; syncs DisableAWSNodeTerminationHandlerAnnotation and mirrors hypershift.openshift.io/aws-termination-handler-queue-url.
NodePool Controllers — Spot support
hypershift-operator/controllers/nodepool/aws.go, hypershift-operator/controllers/nodepool/aws_test.go, hypershift-operator/controllers/nodepool/capi.go, hypershift-operator/controllers/nodepool/capi_test.go
Added AnnotationEnableSpot and isSpotEnabled(), inject termination-handler managed tag when spot enabled, propagate interruptibleInstance label, create/delete spot-specific MachineHealthCheck, and tests verifying spot MHC and labels.
E2E Tests & Utilities
test/e2e/nodepool_spot_termination_handler_test.go, test/e2e/nodepool_test.go, test/e2e/util/aws.go
New e2e SpotTerminationHandlerTest (SQS-driven rebalance flow), added GetSQSClient() helper, and adjusted test harness to disable SharedRole for first cluster in relevant test.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci Bot added area/api Indicates the PR includes changes for the API area/cli Indicates the PR includes changes for CLI area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/platform/aws PR/issue for AWS (AWSPlatform) platform area/testing Indicates the PR includes changes for e2e testing and removed do-not-merge/needs-area labels Jan 22, 2026
@openshift-ci

openshift-ci Bot commented Jan 22, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2026
@enxebre
enxebre force-pushed the spot-with-termination-handler branch 3 times, most recently from 213ba6a to 1e859ca Compare January 23, 2026 00:50
@enxebre

enxebre commented Jan 23, 2026

Copy link
Copy Markdown
Member Author

/test e2e-aws

1 similar comment
@enxebre

enxebre commented Jan 23, 2026

Copy link
Copy Markdown
Member Author

/test e2e-aws

@enxebre

enxebre commented Jan 26, 2026

Copy link
Copy Markdown
Member Author

/test e2e e2e-aws

@enxebre
enxebre requested review from csrwng and sjenning January 26, 2026 10:53
@enxebre
enxebre force-pushed the spot-with-termination-handler branch from 1e859ca to 38bf314 Compare January 26, 2026 11:37
@enxebre

enxebre commented Jan 26, 2026

Copy link
Copy Markdown
Member Author

/test e2e-aws

@csrwng csrwng left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments/questions

grep certificate-authority-data /etc/kubernetes/kubeconfig | awk '{print $2}' | base64 -d > /var/run/secrets/kubernetes.io/serviceaccount/ca.crt

/usr/bin/control-plane-operator token-minter \
--service-account-namespace=openshift-cluster-version \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the openshift-cluster-version SA have the right rbac to be able to drain nodes?

@enxebre enxebre Jan 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeh it's admin. Note the comment in the script though. Hopefully we can get rid of this in future

        # we use openshift-cluster-version/default service account to avoid the need to create a new one.
        # We need to authenticate with a SA because the terminantion handler has hardcoded in cluster client credentials.
        # https://github.com/aws/aws-node-termination-handler/pull/1230


// AnnotationEnableSpot enables spot instance creation for this NodePool.
// When set to "true", the NodePool will create spot instances instead of on-demand instances.
AnnotationEnableSpot = "hypershift.openshift.io/enable-spot"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be defined in the api module?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep them here since this is just to hide this feature while having e2e coverage. The intent is to have a follow up to drop the annotations and expose in the API.


// AnnotationTerminationHandlerQueueURL specifies the SQS queue URL for spot interruption events.
// This annotation is required on the HostedCluster to enable the termination handler component.
AnnotationTerminationHandlerQueueURL = "hypershift.openshift.io/aws-termination-handler-queue-url"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be defined in the api module?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep them here since this is just to hide this feature while having e2e coverage. The intent is to have a follow up to drop the annotations and expose in the API.

t.Run("SpotTerminationHandlerTest", func(t *testing.T) {
controlPlaneNamespace := manifests.HostedControlPlaneNamespace(s.hostedCluster.Namespace, s.hostedCluster.Name)

// Step 0: Add SQS permissions to the CPO role so the termination handler can access the queue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the SQS permissions go in the nodepool role? (they're added there in the cli's iam.go)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are added as well already. I added in the test as well since I believe we had some e2e setup to run with the published managed policies therefore the inline append is needed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep cpo role wasn't intended. Updated.

@enxebre
enxebre force-pushed the spot-with-termination-handler branch 4 times, most recently from 2f63b2a to f6dfa1c Compare January 26, 2026 16:00
@enxebre
enxebre force-pushed the spot-with-termination-handler branch from f6dfa1c to 5234288 Compare January 26, 2026 21:56
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Fix all issues with AI agents
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/secret.go`:
- Around line 11-29: adaptCredentialsSecret can panic if HCP AWS spec or
RolesRef.NodePoolManagementARN is nil/empty; add a defensive guard at the start
of adaptCredentialsSecret to validate hcp.Spec.Platform != nil,
hcp.Spec.Platform.AWS != nil and that
hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN is non-empty, and return a
clear error (not panic) if the ARN is missing; update usage in
adaptCredentialsSecret (referencing hcp and roleARN) to only proceed building
the awsCredentialsTemplate when the checks pass.

In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 4317-4338: The code in isAWSNodeTerminationHandlerNeeded currently
treats any presence of the annotation "hypershift.openshift.io/enable-spot" as
enabling spot handling; change the check to validate the annotation value (e.g.,
use strconv.ParseBool or compare to "true") so only truthy values enable the
handler. In the loop over nodePools in isAWSNodeTerminationHandlerNeeded, fetch
nodePool.Annotations["hypershift.openshift.io/enable-spot"], parse it with
strconv.ParseBool (or normalize and compare to "true"), and only return true
when parsing succeeds and yields true; keep using listNodePools and preserve
existing error handling. Ensure nil annotations are handled safely.

In `@hypershift-operator/controllers/nodepool/capi_test.go`:
- Around line 1740-1744: The test contains a redundant conditional where
expectedTemplateName is initialized to awsMachineTemplateName and then set to
the same value inside the if block using isSpotEnabled(tt.nodePool); remove the
redundant branch or implement the intended alternative value for spot instances.
Update the test by deleting the if block that assigns expectedTemplateName =
awsMachineTemplateName (or replace it with the correct template name for spot
instances), keeping the final assertion
g.Expect(templateList.Items[0].GetName()).To(Equal(expectedTemplateName))
unchanged; reference symbols: expectedTemplateName, awsMachineTemplateName,
isSpotEnabled, tt.nodePool, templateList.Items.

In `@test/e2e/nodepool_spot_termination_handler_test.go`:
- Around line 114-126: The SQS IAM policy constructed in the sqsPolicy string
currently uses a wildcard ARN ("arn:aws:sqs:%s:*:*") granting
ReceiveMessage/DeleteMessage to all queues in the region; change this to target
only the test queue by building the exact queue ARN (or URL-derived ARN) instead
of using the wildcard: obtain the queue name or URL (e.g., via the code that
creates/looks up the queue) and combine it with the AWS account ID and
s.clusterOpts.AWSPlatform.Region to produce the specific ARN, then replace the
Resource value in sqsPolicy with that specific ARN so only the intended queue is
allowed.
- Around line 286-293: The HostedCluster annotation removal must be registered
with defer so it always runs even if the test fails; move the
e2eutil.UpdateObject call that deletes AnnotationTerminationHandlerQueueURL
earlier (immediately after you set or assert the annotation exists) and wrap it
in a defer closure that calls e2eutil.UpdateObject(t, s.ctx, s.mgmtClient,
s.hostedCluster, func(obj *hyperv1.HostedCluster) { delete(obj.Annotations,
AnnotationTerminationHandlerQueueURL) }); then remove the explicit cleanup block
currently in Step 7 so you don't run the same removal twice.
🧹 Nitpick comments (2)
test/e2e/nodepool_test.go (1)

215-219: Consider making the SharedRole requirement more explicit.

The if i == 0 condition couples the SharedRole configuration to the test's position in the slice. If tests are reordered or the SpotTerminationHandler test moves to a different HostedCluster group, this configuration won't apply correctly.

A more robust approach might be to check if the test group contains tests requiring real roles, or add a field to HostedClusterNodePoolTestCases to indicate role requirements.

test/e2e/nodepool_spot_termination_handler_test.go (1)

40-41: Hardcoded queue name reduces test portability.

While the SQS queue URL is now discovered dynamically (addressing the previous account ID exposure concern), the queue name agarcial-nth-queue is still hardcoded. This appears to be a personal/dev queue name and makes the test non-portable across different AWS accounts or environments.

Consider parameterizing via environment variable:

🔧 Suggested fix
-	// testSQSQueueName is the SQS queue name used for testing.
-	testSQSQueueName = "agarcial-nth-queue"
+)
+
+var (
+	// testSQSQueueName is read from environment for testing.
+	testSQSQueueName = os.Getenv("HYPERSHIFT_TEST_SQS_QUEUE_NAME")

Then validate in Setup:

if testSQSQueueName == "" {
    t.Skip("HYPERSHIFT_TEST_SQS_QUEUE_NAME environment variable not set")
}

Comment on lines +11 to +29
func adaptCredentialsSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error {
hcp := cpContext.HCP

// Get the NodePoolManagementARN from the HCP spec.
// The predicate ensures this is set before the component is reconciled.
roleARN := hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN

// Create AWS credentials file content using web identity token
// This follows the same pattern as karpenter operator
awsCredentialsTemplate := `[default]
role_arn = %s
web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token
sts_regional_endpoints = regional
`
credentials := fmt.Sprintf(awsCredentialsTemplate, roleARN)

// Set the credentials in the secret
secret.Data = map[string][]byte{"credentials": []byte(credentials)}
secret.Type = corev1.SecretTypeOpaque

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add a defensive guard for missing AWS spec/role ARN to avoid panic.
This keeps tests or future callers safe if the predicate is bypassed.

🛠️ Suggested fix
 func adaptCredentialsSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error {
 	hcp := cpContext.HCP
+	if hcp == nil || hcp.Spec.Platform.AWS == nil || hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN == "" {
+		return fmt.Errorf("nodePoolManagementARN is required to build aws credentials")
+	}
 
 	// Get the NodePoolManagementARN from the HCP spec.
 	// The predicate ensures this is set before the component is reconciled.
 	roleARN := hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func adaptCredentialsSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error {
hcp := cpContext.HCP
// Get the NodePoolManagementARN from the HCP spec.
// The predicate ensures this is set before the component is reconciled.
roleARN := hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN
// Create AWS credentials file content using web identity token
// This follows the same pattern as karpenter operator
awsCredentialsTemplate := `[default]
role_arn = %s
web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token
sts_regional_endpoints = regional
`
credentials := fmt.Sprintf(awsCredentialsTemplate, roleARN)
// Set the credentials in the secret
secret.Data = map[string][]byte{"credentials": []byte(credentials)}
secret.Type = corev1.SecretTypeOpaque
func adaptCredentialsSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error {
hcp := cpContext.HCP
if hcp == nil || hcp.Spec.Platform.AWS == nil || hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN == "" {
return fmt.Errorf("nodePoolManagementARN is required to build aws credentials")
}
// Get the NodePoolManagementARN from the HCP spec.
// The predicate ensures this is set before the component is reconciled.
roleARN := hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN
// Create AWS credentials file content using web identity token
// This follows the same pattern as karpenter operator
awsCredentialsTemplate := `[default]
role_arn = %s
web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token
sts_regional_endpoints = regional
`
credentials := fmt.Sprintf(awsCredentialsTemplate, roleARN)
// Set the credentials in the secret
secret.Data = map[string][]byte{"credentials": []byte(credentials)}
secret.Type = corev1.SecretTypeOpaque
🤖 Prompt for AI Agents
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/secret.go`
around lines 11 - 29, adaptCredentialsSecret can panic if HCP AWS spec or
RolesRef.NodePoolManagementARN is nil/empty; add a defensive guard at the start
of adaptCredentialsSecret to validate hcp.Spec.Platform != nil,
hcp.Spec.Platform.AWS != nil and that
hcp.Spec.Platform.AWS.RolesRef.NodePoolManagementARN is non-empty, and return a
clear error (not panic) if the ARN is missing; update usage in
adaptCredentialsSecret (referencing hcp and roleARN) to only proceed building
the awsCredentialsTemplate when the checks pass.

Comment thread hypershift-operator/controllers/nodepool/capi_test.go Outdated
Comment on lines +114 to +126
sqsPolicy := fmt.Sprintf(`{
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Action": [
"sqs:ReceiveMessage",
"sqs:DeleteMessage"
],
"Resource": "arn:aws:sqs:%s:*:*"
}
]
}`, s.clusterOpts.AWSPlatform.Region)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Overly permissive SQS IAM policy grants access to all queues in the region.

The policy uses a wildcard resource arn:aws:sqs:%s:*:* which grants ReceiveMessage and DeleteMessage permissions to all SQS queues in the region, not just the test queue. This violates the principle of least privilege.

Scope the policy to the specific queue:

🔒 Suggested fix
 		sqsPolicy := fmt.Sprintf(`{
 			"Version": "2012-10-17",
 			"Statement": [
 				{
 					"Effect": "Allow",
 					"Action": [
 						"sqs:ReceiveMessage",
 						"sqs:DeleteMessage"
 					],
-					"Resource": "arn:aws:sqs:%s:*:*"
+					"Resource": "%s"
 				}
 			]
-		}`, s.clusterOpts.AWSPlatform.Region)
+		}`, getQueueARNFromURL(sqsQueueURL))

You'll need to discover the queue URL before creating the policy, or construct the ARN from the queue name and account ID.

🤖 Prompt for AI Agents
In `@test/e2e/nodepool_spot_termination_handler_test.go` around lines 114 - 126,
The SQS IAM policy constructed in the sqsPolicy string currently uses a wildcard
ARN ("arn:aws:sqs:%s:*:*") granting ReceiveMessage/DeleteMessage to all queues
in the region; change this to target only the test queue by building the exact
queue ARN (or URL-derived ARN) instead of using the wildcard: obtain the queue
name or URL (e.g., via the code that creates/looks up the queue) and combine it
with the AWS account ID and s.clusterOpts.AWSPlatform.Region to produce the
specific ARN, then replace the Resource value in sqsPolicy with that specific
ARN so only the intended queue is allowed.

Comment thread test/e2e/nodepool_spot_termination_handler_test.go
@enxebre

enxebre commented Jan 27, 2026

Copy link
Copy Markdown
Member Author

/test e2e-aws

enxebre and others added 2 commits January 27, 2026 09:08
Implement control plane operator v2 component for AWS node termination
handler with MachineHealthCheck integration.

Components added:
- AWS node termination handler Deployment for spot interruption handling
- AWS credentials secret with NodePoolManagement role ARN
- MachineHealthCheck for spot instances with interruptible-instance label
- Component predicate checking for AWS platform and SQS queue URL annotation
- interruptible-instance label propagation to MachineDeployment templates and existing Machines

The component is deployed when the HostedCluster has:
- AWS platform type
- Annotation: hypershift.openshift.io/aws-termination-handler-queue-url

Features:
- SQS queue monitoring for spot interruption events
- Automatic node draining on spot interruption warnings
- Machine health checking for interrupted instances
- Web identity token authentication using NodePoolManagement ARN

TODOs:
- Replace aws-node-termination-handler image with mirror image or payload once available
- Remove SQS queue URL annotation once the input is in the HostedCluster AWS API
- Replace hardcoded constants with API based configuration

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…and spot

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@enxebre
enxebre force-pushed the spot-with-termination-handler branch from 5234288 to 85008d9 Compare January 27, 2026 08:09
@enxebre

enxebre commented Jan 27, 2026

Copy link
Copy Markdown
Member Author

/verified by e2e

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Jan 27, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@enxebre: This PR has been marked as verified by e2e.

Details

In response to this:

/verified by e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In
`@control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yaml`:
- Around line 84-86: The fixture is missing the AWS_REGION env var value causing
SQS client MissingRegion; update the deployment fixture so the AWS_REGION
environment variable is populated with the AWS region consistent with QUEUE_URL
(us-east-1). Edit the test YAML where the env list contains the AWS_REGION entry
(in the aws-node-termination-handler deployment fixture) and set its value to
"us-east-1" to mirror how awsnodeterminationhandler/deployment.go pulls
hcp.Spec.Platform.AWS.Region, ensuring the rendered deployment matches the
controller logic.

In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 5327-5328: Replace the hardcoded annotation key
"hypershift.openshift.io/enable-spot" in the test Annotations maps with the
existing constant nodepool.AnnotationEnableSpot; add the appropriate import for
the nodepool package (use an import alias if needed) and update all test
occurrences where that string is used so they reference
nodepool.AnnotationEnableSpot instead.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/deployment_test.go (1)

100-116: Missing test coverage for issuerURL / token-audience.

The test sets tc.issuerURL in the HCP spec but never verifies it was correctly applied to the token-minter-kube container's args. This leaves the token-audience string replacement logic untested.

♻️ Proposed addition to verify token-audience
 		g.Expect(regionValue).To(Equal(tc.expectedRegion))
 		g.Expect(queueURLValue).To(Equal(tc.expectedQueueURL))
+
+		// Verify token-audience was set correctly in token-minter-kube container
+		if tc.issuerURL != "" {
+			for _, container := range deployment.Spec.Template.Spec.Containers {
+				if container.Name == "token-minter-kube" {
+					found := false
+					for _, arg := range container.Args {
+						if strings.Contains(arg, "--token-audience="+tc.issuerURL) {
+							found = true
+							break
+						}
+					}
+					g.Expect(found).To(BeTrue(), "expected token-audience to contain issuerURL")
+				}
+			}
+		}

Also add test cases with issuerURL values to exercise this code path.

Comment on lines +84 to +86
- name: AWS_REGION
- name: QUEUE_URL
value: https://sqs.us-east-1.amazonaws.com/123456789012/test-queue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the current state of the fixture file
cat control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yaml

Repository: openshift/hypershift

Length of output: 7410


🏁 Script executed:

#!/bin/bash
# Search for AWS_REGION environment variable setup in controller code
rg -n "AWS_REGION" control-plane-operator/controllers/hostedcontrolplane -g '*.go' -A 3 -B 1

Repository: openshift/hypershift

Length of output: 4408


🏁 Script executed:

#!/bin/bash
# Find the controller file that manages aws-node-termination-handler
fd -e go -path '*hostedcontrolplane*' | xargs rg -l "aws.node.termination.handler|aws-node-termination-handler" | head -5

Repository: openshift/hypershift

Length of output: 672


🏁 Script executed:

#!/bin/bash
# Examine the deployment.go file to understand how awsRegion is used
sed -n '30,70p' control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/deployment.go

Repository: openshift/hypershift

Length of output: 1474


🏁 Script executed:

#!/bin/bash
# Check function signature and how awsRegion/queueURL parameters are passed
rg -n "func.*awsRegion|func.*queueURL" control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/

Repository: openshift/hypershift

Length of output: 46


🏁 Script executed:

#!/bin/bash
# Find where awsRegion variable is defined and initialized
sed -n '1,50p' control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/deployment.go

Repository: openshift/hypershift

Length of output: 1506


🏁 Script executed:

#!/bin/bash
# Search for the function definition that contains this code
rg -n "func.*ReconcileAWSNodeTerminationHandler|func.*Build.*Deployment" control-plane-operator/controllers/hostedcontrolplane/v2/awsnodeterminationhandler/deployment.go -A 30

Repository: openshift/hypershift

Length of output: 46


AWS_REGION must be populated in the rendered deployment.

The controller code in awsnodeterminationhandler/deployment.go correctly sets AWS_REGION from hcp.Spec.Platform.AWS.Region, but the fixture shows it as empty despite QUEUE_URL being set to a us-east-1 queue. Without AWS_REGION, the SQS client initialization will fail with a MissingRegion error. Update the fixture to populate AWS_REGION consistently with the queue region.

Fixture update
-        - name: AWS_REGION
+        - name: AWS_REGION
+          value: us-east-1
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: AWS_REGION
- name: QUEUE_URL
value: https://sqs.us-east-1.amazonaws.com/123456789012/test-queue
- name: AWS_REGION
value: us-east-1
- name: QUEUE_URL
value: https://sqs.us-east-1.amazonaws.com/123456789012/test-queue
🤖 Prompt for AI Agents
In
`@control-plane-operator/controllers/hostedcontrolplane/testdata/aws-node-termination-handler/zz_fixture_TestControlPlaneComponents_aws_node_termination_handler_deployment.yaml`
around lines 84 - 86, The fixture is missing the AWS_REGION env var value
causing SQS client MissingRegion; update the deployment fixture so the
AWS_REGION environment variable is populated with the AWS region consistent with
QUEUE_URL (us-east-1). Edit the test YAML where the env list contains the
AWS_REGION entry (in the aws-node-termination-handler deployment fixture) and
set its value to "us-east-1" to mirror how
awsnodeterminationhandler/deployment.go pulls hcp.Spec.Platform.AWS.Region,
ensuring the rendered deployment matches the controller logic.

Comment on lines +5327 to +5328
Annotations: map[string]string{
"hypershift.openshift.io/enable-spot": "true",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for enable-spot annotation constant definitions
rg -n "enable-spot|EnableSpot" --type=go -g '!*_test.go' -C2

Repository: openshift/hypershift

Length of output: 1598


🏁 Script executed:

#!/bin/bash
# Search for annotation constants in general to understand naming patterns
rg -n "Annotation.*=.*\"hypershift" --type=go -g '!*_test.go' | head -20

Repository: openshift/hypershift

Length of output: 3610


🏁 Script executed:

#!/bin/bash
# Look at the test file to see where the annotation is used
rg -n "enable-spot" --type=go -C3 | grep -A3 -B3 "hostedcluster_controller_test.go"

Repository: openshift/hypershift

Length of output: 2865


🏁 Script executed:

#!/bin/bash
# Check for constant definitions around line 5327-5328 context in test file
fd "hostedcluster_controller_test.go" --type f | xargs wc -l

Repository: openshift/hypershift

Length of output: 147


Use the AnnotationEnableSpot constant instead of hardcoding the annotation key.

The annotation "hypershift.openshift.io/enable-spot" is hardcoded in test cases at lines 5328, 5366, and 5395. A constant AnnotationEnableSpot already exists in the nodepool package (controllers/nodepool/aws.go:31) and should be imported and used here for consistency and maintainability.

🤖 Prompt for AI Agents
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`
around lines 5327 - 5328, Replace the hardcoded annotation key
"hypershift.openshift.io/enable-spot" in the test Annotations maps with the
existing constant nodepool.AnnotationEnableSpot; add the appropriate import for
the nodepool package (use an import alias if needed) and update all test
occurrences where that string is used so they reference
nodepool.AnnotationEnableSpot instead.

@enxebre

enxebre commented Jan 27, 2026

Copy link
Copy Markdown
Member Author

/test e2e-aws

Add e2e test to validate:
- AWS Node Termination Handler deployment is created when SQS annotation is set
- Spot MachineHealthCheck is created with correct interruptible-instance label selector

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@enxebre
enxebre force-pushed the spot-with-termination-handler branch from 85008d9 to ec91275 Compare January 27, 2026 10:37
@openshift-ci-robot openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label Jan 27, 2026
@enxebre

enxebre commented Jan 27, 2026

Copy link
Copy Markdown
Member Author

/verified by e2e

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Jan 27, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@enxebre: This PR has been marked as verified by e2e.

Details

In response to this:

/verified by e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci

openshift-ci Bot commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

@enxebre: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@csrwng

csrwng commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2026
@enxebre

enxebre commented Jan 27, 2026

Copy link
Copy Markdown
Member Author

/override "Red Hat Konflux / hypershift-operator-main-enterprise-contract / hypershift-operator-main"
/override "Red Hat Konflux / hypershift-operator-enterprise-contract / hypershift-operator-main"

@openshift-ci

openshift-ci Bot commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

@enxebre: Overrode contexts on behalf of enxebre: Red Hat Konflux / hypershift-operator-enterprise-contract / hypershift-operator-main, Red Hat Konflux / hypershift-operator-main-enterprise-contract / hypershift-operator-main

Details

In response to this:

/override "Red Hat Konflux / hypershift-operator-main-enterprise-contract / hypershift-operator-main"
/override "Red Hat Konflux / hypershift-operator-enterprise-contract / hypershift-operator-main"

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Indicates the PR includes changes for the API area/cli Indicates the PR includes changes for CLI area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/platform/aws PR/issue for AWS (AWSPlatform) platform area/testing Indicates the PR includes changes for e2e testing jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants